-
Notifications
You must be signed in to change notification settings - Fork 68
disallow manual trigger of completed tasks #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I left one minor comment.
model/task.go
Outdated
// A task is runable when both of these condition are matched | ||
// 1. Its max execution has not reached | ||
// 2. Its expiration time has not reached | ||
func (t *Task) Runable() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure about the naming in go, but to be consistent with the name TaskIsNotRunable
, I’d call this function isRunnable()
. The is
will clearly indicate that this is boolean returning function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me change.
name convention in go:
- anything with lower case initial is private: mean it can only be call from inside the package
- antyhing upper case is public: mean it can be call from outside(the one that import it)
so will change to IsRunable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisli30 update to IsRunable
because
model/tasks.go is in package model
It was being call in package taskengine
so it need to be IsRunable
This isn't just a convention in term of syntax. It's the language spec https://go.dev/ref/spec#Exported_identifiers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, understood and sounds good!
Fixes #92